Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A handful of interface changes to support OpenXR future rebasing #1846

Merged
merged 8 commits into from
Oct 31, 2024

Conversation

jzulauf-lunarg
Copy link
Contributor

@jzulauf-lunarg jzulauf-lunarg commented Oct 29, 2024

Add needed decoder and interface extensions
Add interface to get/set capture mode
Expose dispatch table getters
Add initialize to StructPointerDecoder allocation
Add vulkan guard to consumer and decode variables
Split OverrideCreateInstance/Device
Add VkPhysicalDevice aliasing support
Reorganize common options

Not sure if these will interfere with @panos-lunarg pending changes, will wait if needed

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 290663.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5232 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5232 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291019.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5237 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5237 passed.

Copy link
Contributor

@MarkY-LunarG MarkY-LunarG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, but otherwise LGTM


GFXRECON_BEGIN_NAMESPACE(gfxrecon)
GFXRECON_BEGIN_NAMESPACE(decode)
template <typename T, typename = void>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Put a blank line above this.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291462.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5244 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5244 passed.

Copy link
Contributor Author

@jzulauf-lunarg jzulauf-lunarg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidd-lunarg , @bradgrantham-lunarg -- this change, particularly, I'd like your review of

const StructPointerDecoder<Decoded_VkInstanceCreateInfo>* pCreateInfo,
const StructPointerDecoder<Decoded_VkAllocationCallbacks>* pAllocator,
HandlePointerDecoder<VkInstance>* pInstance)
void VulkanReplayConsumerBase::ModifyCreateInstanceInfo(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@davidd-lunarg -- @bradgrantham-lunarg ... starting here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it keeps the original functionality but split into separate pieces. Looks OK to me.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291493.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5245 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5245 passed.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 291531.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5246 running.

@jzulauf-lunarg
Copy link
Contributor Author

That's all folks.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5246 passed.

return output_data_;
}

typename T::struct_type* AllocateOutputData(size_t len, const typename T::struct_type& init)
{
assert(!DecoderHasOutputAllocator<T>::value);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer GFXRECON_ASSERT to assert. The CMake option GFXRECON_ENABLE_RELEASE_ASSERTS can be enabled to log failed GFXRECON_ASSERTs as errors in release builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of this file is all assert(...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. For new code please prefer GFXRECON_ASSERT. There may come a day when we change as many assert as we can to GFXRECON_ASSERT

const StructPointerDecoder<Decoded_VkInstanceCreateInfo>* pCreateInfo,
const StructPointerDecoder<Decoded_VkAllocationCallbacks>* pAllocator,
HandlePointerDecoder<VkInstance>* pInstance)
void VulkanReplayConsumerBase::ModifyCreateInstanceInfo(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it keeps the original functionality but split into separate pieces. Looks OK to me.

@@ -68,12 +92,13 @@ class StructPointerDecoder : public PointerDecoderBase
typename T::struct_type* AllocateOutputData(size_t len)
{
output_len_ = len;
output_data_ = DecodeAllocator::Allocate<typename T::struct_type>(len);
output_data_ = StructPointerOutputDataAllocator(decoded_structs_, len);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change for a new use of StructPointerDecoder? Or is there an existing use that wasn't being handled correclty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New use.

@bradgrantham-lunarg bradgrantham-lunarg removed their request for review October 31, 2024 15:57
@bradgrantham-lunarg
Copy link
Contributor

If @davidd-lunarg and @MarkY-LunarG approve the PR I trust their approval.

For layered API support.
For layered API support.
Prevents uninitialized next/pNext.  The lower level decoders aren't
always setting values. Set a good state at initialization (zeros)

Layered API support.
Prevent collisions with other API consumers and decoders

Layered API support.
Split vulkan instance and device creation override into reusable
pieces suitable for layered API support (specifically OpenXR)

Make neeed information public for other replay consumers

Layered API support.
Vulkan captures unwrapped physical device handles, Layered API (like OpenXR)
captures wrapped handles.  During replay two HandleId's will reference the
same VkPhysical device.  The vulkan_alias is the handleId as known by the
vulkan_consumer, which will be created/updated, etc, by all Vulkan replay
calls.
Move replay options around that are shared between multiple
replay paths.

For layered API support.
@ci-tester-lunarg
Copy link

CI gfxreconstruct build queued with queue ID 292063.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5252 running.

@ci-tester-lunarg
Copy link

CI gfxreconstruct build # 5252 passed.

@jzulauf-lunarg jzulauf-lunarg merged commit 837e74f into LunarG:dev Oct 31, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants